Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Storybook] Improve typing, add doc links & remove redundant JSDoc in preview.tsx #11745

Merged
merged 10 commits into from
Dec 8, 2024

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Dec 5, 2024

- Migrate v6-style export to Storybook 7 default export (as per https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#default-export-in-previewjs)
- Remove redundant JS-style type import
- add typing for preview config object
- add doc link for globalTypes config
@Philzen Philzen marked this pull request as ready for review December 5, 2024 10:00
@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

I did a little test an am getting the impression this file is not being used at all atm.

Tried deleting storybook from my package.json and then tried setting up on RW v7 both yarn rw sbv (which only creates main.ts and preview-body.html in web/.storybook) and yarn rw sb (which strangely does not create a web/config folder at all).

Looking at Philzen@8fb0f86 … is it possible that configureStorybook.js can well be deleted and the template for preview.tsx should best be moved over to cli-packages/storybook-vite instead?

@Josh-Walker-GM

@Tobbe
Copy link
Member

Tobbe commented Dec 5, 2024

@arimendelow Could you please have a look at @Philzen's latest comment here? About removing configureStorybook.js

- Migrate v6-style export to Storybook 7 default export (as per https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#default-export-in-previewjs)
- Remove redundant JS-style type import
- add typing for preview config object
- add doc link for globalTypes config
@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

@arimendelow Could you please have a look at @Philzen's latest comment here? About removing configureStorybook.js

Ah nevermind about this one – at least i can see that this file is still required for https://github.com/redwoodjs/redwood/blob/v8.4.1/packages/cli/src/commands/setup/ui/libraries/chakra-ui.js#L10

However the question about the preview.tsx template remains. I can see it being referenced in configureStorybook.js – although i'm not sure in which scenario it would actually be required (b/c setup chakra has its own template for instance).

Still it may be a good idea to copy (instead of moving) it over to cli-packages/storybook-vite for a turnkey experience.

…ok 7 style

- Migrate v6-style export to Storybook 7 default export (as per https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#default-export-in-previewjs)
- Remove redundant JS-style type import
- add typing for preview config object
- add doc link for decorator config
…k 7 style

- Migrate v6-style export to Storybook 7 default export (as per https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#default-export-in-previewjs)
- Remove redundant JS-style type import
- add typing for preview config object
- add doc link for decorators config
- shorten decorators component
Was otherwise violating (import/order) rule:
- `react-i18next` import should occur after type import of `@storybook/react`
- There should be at least one empty line between import groups
@arimendelow
Copy link
Contributor

Hi all! configureStorybook.js is part of the old SB-Webpack integration, and no longer serves any purpose. Back before Storybook Framework packages, we basically had the Storybook config split between the project and the Redwood Storybook package — configureStorybook is what allowed the user to do some level of customizing. Its purpose is to merge the two configs into one.

This need no longer exists. The ChakraUI integration needs to be updated :)

On the preview.tsx template, it's not required OOTB, and I felt the Storybook docs were a sufficient guide to create it, but we certainly can start generating it on setup! Like you said, we'd move it to here: https://github.com/redwoodjs/redwood/tree/v8.4.1/packages/cli-packages/storybook-vite/src/commands/templates

And then add a block like this one for creating it in the project: https://github.com/redwoodjs/redwood/blob/85511daa303ec4c135339e3cc0865ee149ece145/packages/cli-packages/storybook-vite/src/commands/storybookHandler.ts#L98C1-L116C4

@arimendelow
Copy link
Contributor

So @Philzen you are correct — the file is no longer being used, and these all need to be updated to work with the new integration — it's not just the formatting that needs to change.

@Philzen
Copy link
Contributor Author

Philzen commented Dec 8, 2024

@arimendelow many thanks for chiming in!

configureStorybook.js is part of the old SB-Webpack integration, and no longer serves any purpose.

Still, from looking at https://github.com/redwoodjs/redwood/blob/v8.4.1/packages/cli/src/commands/setup/ui/libraries/chakra-ui.js#L10 it seems still required – maybe that can be refactored so all SB-Webpack related stuff can be deleted (as i understand with RW 8 webpack is gone for good)?

but we certainly can start generating it on setup!

I'd very much support that – gives much more of a turnkey experience. Thanks for outlining the steps required – however as i currently don't have a full local Redwood dev setup (and also given the above point where it still seems used) i'm a bit anxious making these changes myself w/o being able to fully test them.

Hence my suggestion would be to

  1. merge this PR as it is for now (as it is really not changing a lot except formatting and typing of the existing templates).
  2. then you could go forward and cleanup old webpack-related scripts and include the preview.tsx template in storybook-vite

@Philzen
Copy link
Contributor Author

Philzen commented Dec 8, 2024

@arimendelow also i found these

const PATH_WEB_DIR_CONFIG_STORYBOOK_CONFIG = 'web/config/storybook.config.js'
const PATH_WEB_DIR_CONFIG_STORYBOOK_PREVIEW = 'web/config/storybook.preview' // .js, .tsx
const PATH_WEB_DIR_CONFIG_STORYBOOK_MANAGER = 'web/config/storybook.manager.js'

which look like they also need to be updated (i believe manager was also dropped in Storybook 7).

@Tobbe Tobbe added the release:feature This PR introduces a new feature label Dec 8, 2024
@Tobbe Tobbe added this to the next-release milestone Dec 8, 2024
@Tobbe
Copy link
Member

Tobbe commented Dec 8, 2024

Hence my suggestion would be to

  1. merge this PR as it is for now

Sounds good to me. I'll add a changeset and merge it

@Tobbe Tobbe merged commit 455bae8 into redwoodjs:main Dec 8, 2024
49 checks passed
@Tobbe
Copy link
Member

Tobbe commented Dec 8, 2024

@Philzen Can you please write up an issue with the remaining tasks?

@Philzen
Copy link
Contributor Author

Philzen commented Dec 8, 2024

@Philzen Can you please write up an issue with the remaining tasks?

🫡 → #11760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants